• 📝 Generate docstrings

• X
• Mastodon
• Reddit
• LinkedIn

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

🏁 Script executed:

#!/bin/bash
# Read the Zig wrapper implementations and related files
echo "=== zigcc.go ==="
cat scripts/zigcc/zigcc.go

echo -e "\n=== wrapper.py ==="
cat wrapper.py

echo -e "\n=== sandbox.py (relevant sections) ==="
cat scripts/sandbox.py

echo -e "\n=== Makefile (Zig installation section) ==="
grep -A 30 "ZIG_VERSION" Makefile | head -50

echo -e "\n=== zigcc tests ==="
cat scripts/zigcc/zigcc_test.go

echo -e "\n=== Dockerfile changes ==="
cat runtimes/minimal/ubi9-python-3.12/Dockerfile.cpu

Length of output: 16240

@jiridanek: I'll provide a comprehensive analysis of the Zig cross-compilation approach with improvement suggestions.

🎯 Overall Commentary

The Zig cc approach is innovative and pragmatic for cross-compiling to s390x from x86_64/arm64 hosts. Zig's ability to act as a drop-in C/C++ compiler with built-in cross-compilation is well-suited for this use case. However, the implementation has several areas that need refinement.

🔍 Current Architecture Analysis

Strengths:

• ✅ Avoids needing a full cross-compilation toolchain (gcc, binutils, etc.)
• ✅ Zig provides consistent behavior across platforms
• ✅ Handles the problematic -Wp, preprocessor flags
• ✅ syscall.Exec approach (Go) is efficient - no subprocess overhead

Weaknesses:

• ❌ Duplicate wrapper implementations (Go + Python)
• ❌ Hardcoded paths and targets throughout
• ❌ Incomplete argument transformation in Python wrapper
• ❌ Limited error handling and diagnostics
• ❌ Lack of documentation

📋 Specific Improvement Suggestions

1. Choose One Wrapper Implementation

You have both zigcc.go and wrapper.py doing similar jobs. Recommendation: Keep the Go implementation.

Reasons:

• More complete -Wp flag handling (splits on commas properly)
• Better performance (compiled binary vs Python interpreter)
• Uses syscall.Exec - no subprocess overhead
• Already has tests

Action: Remove wrapper.py or document why both exist.

2. Fix Incomplete -Wp Handling in Python Wrapper

The Python wrapper only handles -Wp,-D specifically:

if arg.startswith("-Wp,-D"):
    args.append(arg.replace("-Wp,-D", "-D", 1))

But the Go version correctly handles any -Wp, flag:

if strings.HasPrefix(arg, "-Wp,") {
    newArgs = append(newArgs, strings.Split(arg, ",")[1:]...)
}

Example failure case: -Wp,-DNDEBUG,-D_FORTIFY_SOURCE=2 won't work in Python wrapper.

3. Make Paths and Targets Configurable

Currently hardcoded everywhere:

• /mnt/zig path
• s390x-linux-gnu.2.34 target

Suggestions:

// zigcc.go - use environment variables with defaults
zigPath := getEnvOrDefault("ZIG_PATH", "/mnt/zig")
target := getEnvOrDefault("ZIG_TARGET", "s390x-linux-gnu.2.34")

# sandbox.py - parameterize
def buildinputs(
    dockerfile: pathlib.Path | str,
    platform: str,
    zig_version: str = "0.15.1",
    zig_target: str = "s390x-linux-gnu.2.34"
) -> list[pathlib.Path]:

4. Fix Makefile Architecture Detection

The Makefile references $(ARCH) but I don't see where it's defined in the grep output:

# Add this near the top of Makefile
ARCH_RAW := $(shell uname -m)
ARCH := $(shell echo $(ARCH_RAW) | sed -e 's/aarch64/arm64/' -e 's/x86_64/amd64/')

Also, the Zig download uses $(ARCH) but Zig's download URLs use different conventions (x86_64, aarch64, not amd64/arm64).

5. Improve Makefile Target

The bin/zig-$(ZIG_VERSION) target has issues:

bin/zig-$(ZIG_VERSION):
	@echo "Installing Zig $(ZIG_VERSION)..."
	TMPDIR=$(shell mktemp -d)  # ❌ Wrong - shell executes at parse time
	wget --progress=dot:giga ...

Fixed version:

bin/zig-$(ZIG_VERSION):
	@echo "Installing Zig $(ZIG_VERSION)..."
	$(eval TMPDIR := $(shell mktemp -d))
	wget --progress=dot:giga https://ziglang.org/download/$(ZIG_VERSION)/zig-$(ARCH)-linux-$(ZIG_VERSION).tar.xz
	tar -xJf zig-$(ARCH)-linux-$(ZIG_VERSION).tar.xz -C $(TMPDIR) --strip-components=1
	rm -rf zig-$(ARCH)-linux-$(ZIG_VERSION).tar.xz
	mkdir -p bin
	mv $(TMPDIR) bin/zig-$(ZIG_VERSION)
	chmod +x bin/zig-$(ZIG_VERSION)/zig
	@echo "Zig installed successfully"

6. Remove Shell Wrapper Creation from Makefile

The Makefile creates shell wrappers that are immediately replaced by the Go binary:

# These are overwritten by sandbox.py later
printf '%s\n' '#!/bin/sh' 'exec /mnt/zig cc ...' > $$TMPDIR/zig-cc

Recommendation: Remove these and let sandbox.py handle wrapper installation exclusively.

7. Add Proper Error Handling

In zigcc.go:

func main() {
    // Add validation
    if _, err := os.Stat("/mnt/zig"); os.IsNotExist(err) {
        fmt.Fprintf(os.Stderr, "Error: Zig binary not found at /mnt/zig\n")
        fmt.Fprintf(os.Stderr, "Ensure Zig is mounted correctly\n")
        os.Exit(1)
    }
    
    // Log what we're doing (when verbose flag set)
    if os.Getenv("ZIG_WRAPPER_VERBOSE") != "" {
        fmt.Fprintf(os.Stderr, "Executing: %v\n", newArgs)
    }
    
    // ... rest of code
}

8. Improve Test Coverage

Current test only covers -Wp processing. Add tests for:

func TestProcessArg0(t *testing.T) {
    tests := []struct {
        arg0     string
        expected string
        wantErr  bool
    }{
        {"zig-cc", "cc", false},
        {"zig-c++", "c++", false},
        {"invalid", "", true},
    }
    // ...
}

func TestFullExecution(t *testing.T) {
    // Test with mock Zig binary
    // Verify argument order and transformation
}

9. Add Documentation

Create scripts/zigcc/README.md:

# Zig CC Wrapper

This wrapper enables cross-compilation to s390x using Zig as a drop-in replacement for gcc/g++.

## How It Works

1. The wrapper is invoked as `zig-cc` or `zig-c++`
2. It transforms arguments (especially -Wp flags) for Zig compatibility
3. It executes Zig with the target architecture set to s390x

## Known Limitations

- Zig doesn't support `-Wp,` preprocessor flags, so we split them
- Target is currently hardcoded to `s390x-linux-gnu.2.34`

## Building



## Testing

10. Handle Commented-Out Code in sandbox.py

Remove or document the commented-out CC/CXX environment settings:

# f"--env=CC=/mnt/zig cc -target {target}",  # Why commented out?
# f"--env=CXX=/mnt/zig c++ -target {target}",

Recommendation: Either remove them or add a comment explaining the decision:

# CC/CXX are unset instead of pointing to wrappers because:
# - Some build systems detect and cache the compiler
# - Letting them auto-detect ensures wrapper is found via PATH

11. Remove Debug Logs from Production Code

The large triple-quoted strings in sandbox.py containing build logs should be removed:

"""
Downloading jedi
  × Failed to build `pyzmq==27.1.0`
...
"""

These are useful for debugging but don't belong in production code. Move to a separate debug log file or remove entirely.

12. Consider Zig Version

You're using Zig 0.15.1. Check if there's a newer stable version that might have better compatibility or bug fixes:

13. Add Dockerfile Improvement

The RUN ls /mnt && true line seems like a debug command:

RUN ls /mnt && true

Recommendation: Remove this or add a comment explaining why it's needed. If it's verifying mount availability, make it more explicit:

# Verify /mnt is available for Zig toolchain mounting
RUN test -d /mnt || (echo "Error: /mnt directory not available" && exit 1)

🎯 Priority Recommendations

High Priority:

1. Choose one wrapper implementation (Go recommended)
2. Fix Makefile TMPDIR and ARCH issues
3. Remove debug code and logs from production
4. Add basic documentation

Medium Priority:
5. Make paths/targets configurable
6. Improve error handling and diagnostics
7. Expand test coverage

Low Priority:
8. Check for newer Zig version
9. Add verbose/debug mode
10. Consider additional architecture targets

🤔 Architectural Questions

1. Why not use CC/CXX environment variables? Setting them to the wrapper paths would be more standard than unsetting them.
2. Why mount at /mnt? This is typically for external filesystems. Consider /opt/zig or /usr/local/zig.
3. Why glibc 2.34? The comment mentions "target glibc 2.28 or newer" but uses 2.34. Document this decision.
4. How does this work with GitHub Actions? The PR title mentions "mount /mnt on github actions" - what's the constraint there?

Would you like me to elaborate on any of these suggestions or help implement specific improvements?

🏁 Script executed:

make build

Length of output: 42

🏁 Script executed:

make test

Length of output: 42

🏁 Script executed:

#!/bin/bash
# Check latest Zig version
curl -s https://ziglang.org/download/index.json | jq -r '.master.version, .latest.version' 2>/dev/null || echo "Could not fetch Zig versions"

Length of output: 30

🏁 Script executed:

#!/bin/bash
# Check Zig download page structure to understand versioning
curl -s https://ziglang.org/download/index.json | jq 'keys' 2>/dev/null
echo "---"
# Get info about 0.15.1 specifically
curl -s https://ziglang.org/download/index.json | jq '.["0.15.1"] // empty' 2>/dev/null | head -20
echo "---"
# Check what stable versions are available
curl -s https://ziglang.org/download/index.json | jq 'to_entries | map(select(.key | test("^[0-9]+\\.[0-9]+\\.[0-9]+$"))) | .[].key' 2>/dev/null | sort -V | tail -5

Length of output: 1148

🏁 Script executed:

#!/bin/bash
# Check if 0.15.2 has any relevant improvements
curl -s https://ziglang.org/download/0.15.2/release-notes.html 2>/dev/null | grep -A 5 -i "bug\|fix\|cross\|compile\|target" | head -30
echo "---"
# Also check what architectures are available for 0.15.2
curl -s https://ziglang.org/download/index.json | jq '.["0.15.2"] | keys' 2>/dev/null

Length of output: 518